fix: group charter login #298
Conversation
WalkthroughThe login screen component is redesigned with a new vertical layout, adding a header with a logo, title, and subtitle. The login card becomes a translucent panel. Existing login functionality and component exports remain unchanged. Minor formatting and whitespace updates occur; a class vs. className attribute appears in the new header paragraph. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
platforms/group-charter-manager/src/components/auth/login-screen.tsx (2)
38-71: Ensure SSE is closed on unmount to avoid leaksIf the component unmounts before a message/error, the EventSource stays open. Store it in a ref and close in a cleanup function.
Proposed minimal refactor:
-import { useEffect, useState } from "react"; +import { useEffect, useState, useRef } from "react"; @@ const router = useRouter(); + const eventSourceRef = useRef<EventSource | null>(null); @@ useEffect(() => { initializeAuth(); + return () => { + eventSourceRef.current?.close(); + eventSourceRef.current = null; + }; }, []); @@ const watchEventStream = (sessionId: string) => { @@ - const eventSource = new EventSource(sseUrl); + const eventSource = new EventSource(sseUrl); + eventSourceRef.current = eventSource; @@ - eventSource.close(); + eventSource.close(); + eventSourceRef.current = null; @@ console.error('EventSource error:', error); - eventSource.close(); + eventSource.close(); + eventSourceRef.current = null;
39-41: Require explicit base URL or default to same-origin to avoid mixed-content/CORS issuesBoth
apiClient.tsandlogin-screen.tsxcurrently fall back tohttp://localhost:3001whenNEXT_PUBLIC_GROUP_CHARTER_BASE_URLis unset. In production on HTTPS, this leads to mixed-content and CORS failures. Please:
- Remove the hardcoded
http://localhost:3001fallback and default to the same origin (e.g.window.location.origin) or use a relative base path.- Document and include
NEXT_PUBLIC_GROUP_CHARTER_BASE_URLin.env.example(and README) so it’s always set in prod.Files to update:
- platforms/group-charter-manager/src/lib/apiClient.ts (line 15)
- platforms/group-charter-manager/src/components/auth/login-screen.tsx (lines 39–41)
Suggested diff:
--- a/src/lib/apiClient.ts - baseURL: process.env.NEXT_PUBLIC_GROUP_CHARTER_BASE_URL || 'http://localhost:3001', + baseURL: process.env.NEXT_PUBLIC_GROUP_CHARTER_BASE_URL ?? window.location.origin, --- a/src/components/auth/login-screen.tsx - const baseUrl = process.env.NEXT_PUBLIC_GROUP_CHARTER_BASE_URL || 'http://localhost:3001'; + const baseUrl = process.env.NEXT_PUBLIC_GROUP_CHARTER_BASE_URL ?? window.location.origin;Add to
.env.example:NEXT_PUBLIC_GROUP_CHARTER_BASE_URL=https://your-domain.com
🧹 Nitpick comments (4)
platforms/group-charter-manager/src/components/auth/login-screen.tsx (4)
93-101: Improve LCP for the logo imageAbove-the-fold brand image can be marked as priority to reduce LCP and avoid lazy loading.
Apply this diff:
<Image src="/logo.png" alt="Group Charter Manager Logo" width={50} height={50} + priority />
110-112: Avoid multiple H1s on a single pageThere are two h1 elements (Lines 102 and 110). Use a single H1 and demote the card title to H2 for better semantics/a11y.
Apply this diff:
- <h1 className="text-2xl font-bold text-gray-900 mb-2"> + <h2 className="text-2xl font-bold text-gray-900 mb-2"> Group Charter Manager - </h1> + </h2>Also applies to: 102-102
108-108: Improve legibility of translucent cardAdd a backdrop blur to enhance readability over varied backgrounds when using bg-white/50.
Apply this diff:
- <div className="bg-white/50 p-8 rounded-lg shadow-lg max-w-md w-full"> + <div className="bg-white/50 backdrop-blur-md p-8 rounded-lg shadow-lg max-w-md w-full">
54-56: Re-evaluate storing tokens in localStoragePersisting auth tokens in localStorage increases XSS blast radius. Consider HttpOnly, Secure cookies with SameSite and short-lived access tokens + refresh flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platforms/group-charter-manager/src/components/auth/login-screen.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
platforms/group-charter-manager/src/components/auth/login-screen.tsx (2)
platforms/eVoting/src/lib/authUtils.ts (2)
setAuthId(18-20)setAuthToken(5-7)platforms/pictique/src/lib/types.ts (1)
Image(73-76)
🔇 Additional comments (1)
platforms/group-charter-manager/src/components/auth/login-screen.tsx (1)
151-158: LGTM: clear, user-friendly info panelNice touch explaining W3DS and data sovereignty. Readable and consistent with the new visual style.
| eventSource.onmessage = (e) => { | ||
| const data = JSON.parse(e.data); | ||
| console.log('Auth data received:', data); | ||
|
|
||
| const { user, token } = data; | ||
|
|
||
| // Set authentication data | ||
| setAuthId(user.id); | ||
| setAuthToken(token); | ||
|
|
||
| // Close the event source | ||
| eventSource.close(); | ||
|
|
||
| // Set authenticating state | ||
| setIsAuthenticating(true); | ||
|
|
||
| // Force a page refresh to trigger AuthProvider re-initialization | ||
| window.location.reload(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden SSE handler: guard parsing, validate payload, and close on error
A malformed/heartbeat SSE event will throw on JSON.parse and leave the stream open. Add try/catch and validate the expected shape before using it.
Apply this diff:
- eventSource.onmessage = (e) => {
- const data = JSON.parse(e.data);
- console.log('Auth data received:', data);
-
- const { user, token } = data;
-
- // Set authentication data
- setAuthId(user.id);
- setAuthToken(token);
-
- // Close the event source
- eventSource.close();
-
- // Set authenticating state
- setIsAuthenticating(true);
-
- // Force a page refresh to trigger AuthProvider re-initialization
- window.location.reload();
- };
+ eventSource.onmessage = (e) => {
+ try {
+ const data = JSON.parse(e.data);
+ console.log('Auth data received:', data);
+ const { user, token } = data || {};
+ if (!user?.id || !token) {
+ console.warn('Incomplete auth payload, ignoring:', data);
+ return;
+ }
+ setIsAuthenticating(true);
+ setAuthId(user.id);
+ setAuthToken(token);
+ eventSource.close();
+ } catch (err) {
+ console.error('Failed to parse auth event:', err);
+ eventSource.close();
+ }
+ // Force a page refresh to trigger AuthProvider re-initialization
+ window.location.reload();
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| eventSource.onmessage = (e) => { | |
| const data = JSON.parse(e.data); | |
| console.log('Auth data received:', data); | |
| const { user, token } = data; | |
| // Set authentication data | |
| setAuthId(user.id); | |
| setAuthToken(token); | |
| // Close the event source | |
| eventSource.close(); | |
| // Set authenticating state | |
| setIsAuthenticating(true); | |
| // Force a page refresh to trigger AuthProvider re-initialization | |
| window.location.reload(); | |
| }; | |
| eventSource.onmessage = (e) => { | |
| try { | |
| const data = JSON.parse(e.data); | |
| console.log('Auth data received:', data); | |
| const { user, token } = data || {}; | |
| if (!user?.id || !token) { | |
| console.warn('Incomplete auth payload, ignoring:', data); | |
| return; | |
| } | |
| setIsAuthenticating(true); | |
| setAuthId(user.id); | |
| setAuthToken(token); | |
| eventSource.close(); | |
| } catch (err) { | |
| console.error('Failed to parse auth event:', err); | |
| eventSource.close(); | |
| } | |
| // Force a page refresh to trigger AuthProvider re-initialization | |
| window.location.reload(); | |
| }; |
🤖 Prompt for AI Agents
In platforms/group-charter-manager/src/components/auth/login-screen.tsx around
lines 47 to 65, the SSE onmessage handler currently calls JSON.parse and assumes
the payload shape which can throw or lead to misuse on malformed/heartbeat
events; wrap parsing in a try/catch, treat empty or non-JSON data as a heartbeat
and ignore it, validate that the parsed object has the expected user and token
properties (user.id and token) before calling setAuthId/setAuthToken, and ensure
eventSource.close() is invoked both on successful authentication and in the
catch/error path; also log parse/validation errors and avoid calling
window.location.reload() unless authentication was successfully set.
| <p class="text-gray-600"> | ||
| Coordinate your group in the MetaState | ||
| </p> |
There was a problem hiding this comment.
Fix JSX attribute: use className, not class
React/TSX doesn’t recognize the HTML class attribute. This will error at compile/runtime.
Apply this diff:
- <p class="text-gray-600">
+ <p className="text-gray-600">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <p class="text-gray-600"> | |
| Coordinate your group in the MetaState | |
| </p> | |
| <p className="text-gray-600"> | |
| Coordinate your group in the MetaState | |
| </p> |
🤖 Prompt for AI Agents
In platforms/group-charter-manager/src/components/auth/login-screen.tsx around
lines 104 to 106, the JSX uses the HTML attribute "class" which React/TSX
doesn't accept; change the attribute to "className" on the <p> element (and any
other JSX elements in that range) so it compiles correctly (e.g., replace
class="text-gray-600" with className="text-gray-600").
Description of change
fixes styling of group charter's login screen
Issue Number
294 but doesnt close
Type of change
How the change has been tested
manual
Change checklist
Summary by CodeRabbit